perf: avoid rebuilding the eligible-worker snapshot per pending task in HttpRemoteTaskRunner by Anubhav-Roy · Pull Request #20299 · apache/druid

Anubhav-Roy · 2026-09-08T13:12:22Z

Fixes #20295.

Description

Under a large pending-task backlog with saturated workers, the Overlord's httpRemote
task runner stalls: one pending-task-runner thread holds statusLock while deep in
worker-snapshot reconstruction, blocking new task submission, status updates, and
worker-sync operations cluster-wide. Restarting doesn't help because the active task set is
reloaded from metadata, the backlog reappears, and the loop re-enters the same
lock-holding scan.

Fixed the Overlord stall on a large pending-task backlog

In HttpRemoteTaskRunner.pendingTasksExecutionLoop(), the loop holds the single
statusLock while iterating every pending task, and for each task calls
findWorkerToRunTask(Task), which rebuilds a full immutable snapshot of all workers via
getWorkersEligibleToRunTasks(). This makes the loop cost
O(pendingTasks × workers × tasksAnnouncedPerWorker), while it holds the lock.

This change computes the snapshot once per pass,
inside synchronized (statusLock) before iterating, and passes it into a new overload
findWorkerToRunTask(Task, ImmutableMap<String, ImmutableWorkerInfo> eligibleWorkers).
The existing findWorkerToRunTask(Task) is retained and now delegates to the overload,
so no other call site changes behavior.

This reduces per-pass cost to O(workers × tasksPerWorker) with no behavioral change:
worker selection within a pass is identical because the input snapshot is identical to
what each per-task call would have recomputed.

Release note

Fixed an issue where the Overlord using the httpRemote task runner could stall for
extended periods (holding statusLock) when a large backlog of pending tasks
accumulated while workers were saturated, blocking task submission and status updates.

Key changed/added classes in this PR

HttpRemoteTaskRunner

This PR has:

been self-reviewed.
- using the concurrency checklist.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.

FrankChen021

Severity	Findings
P0	0
P1	0
P2	2
P3	0
Total	2

Reviewed 1 of 1 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021

Follow-up assessment

The first finding, concerning an unnecessary eligible-worker snapshot when the pending queue is empty, is resolved in the current head. The remaining stale-capacity finding was handled in inline reply 4004995846; no additional inline finding is needed from this follow-up. Reviewed 1 of 1 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

FrankChen021

🟡 Changes recommended

The current head still has a race between fresh worker selection and reservation: worker task announcements can update the worker snapshot outside statusLock, and the assignment path does not revalidate capacity after the selection. The new pre-filter also leaves the cached snapshot stale after a failed fresh selection, which can restore the original repeated snapshot-rebuild lock stall during a worker-capacity transition. Both issues are included as inline findings.

Reviewed 1 of 1 changed files. Static review covered the changed scheduling loop, WorkerHolder snapshot publication, worker-selection strategies, the reservation and assignment path, and the existing review discussion. No tests or build commands were run.

Severity	Findings
P0	0
P1	0
P2	2
P3	0
Total	2

This is an automated review by Codex GPT-5.6 Luna(Max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

Anubhav-Roy · 2026-09-16T12:32:41Z

@kfaraz this fixes an Overlord statusLock stall under a large pending-task backlog, that we faced in Prod.

It touches only the HttpRemoteTaskRunner worker selection.

Would appreciate a review when you have a chance. Automated feedback from @FrankChen021 has been addressed.

(Adding you since I could see you had touched these files.)

FrankChen021 · 2026-09-16T12:41:15Z

Thanks for reporting and submitting a patch. Could you check if above two findings are true or not? If they are not worthy of fixing, you can reply to these comments and AI will re-check the source code with your justification to make a judge.

Anubhav-Roy · 2026-09-16T15:14:42Z

They are not regressions from this code. Addressed and resolved

FrankChen021

Follow-up assessment

I reviewed 1 of 1 changed files at the current head, with the scheduling loop checked against WorkerHolder snapshot publication, all findWorkerToRunTask call sites, selection strategies, reservation, assignment, and lifecycle notifications. No new PR-caused findings remain.

The selection-to-reservation capacity race in the first thread is unchanged from master. The stale-cache path in the second thread can still fall back to master's existing per-task rebuild after a worker-state transition, but this PR does not introduce that behavior; in its targeted saturated-worker case it avoids the repeated snapshot reconstruction. Both supplied threads are therefore resolved for this PR, and no inline reply is warranted.

Validation: git diff --check passed. No tests, builds, dependency installs, formatters, or long validations were run.

This is an automated review by Codex GPT-5.6-Luna(max)

Anubhav-Roy · 2026-09-22T13:23:35Z

Only the docker-tests fails, but I built the branch locally on JDK 25 and ran the embedded test that actually covers this code path, HttpRemoteTaskRunnerWorkerFailTest, and it passes (1 run, 0 failures). The build is clean too.

FrankChen021 · 2026-09-30T06:16:05Z

looks like unit tests were not triggered, can you rebase the branch to the latest master branch?

Anubhav-Roy · 2026-09-30T09:21:53Z

rebased and updated the PR

FrankChen021

🟢 Approval recommended

The lazy per-pass snapshot removes the repeated worker/task-announcement reconstruction from the saturated pending-task scan while preserving the existing fresh worker selection immediately before reservation. Empty and non-schedulable queues do not materialize the snapshot, and the current worker-selection call sites, WorkerHolder snapshot publication, reservation path, and lifecycle notifications were reviewed. The previously reported PR-specific issues are addressed; the remaining selection-to-reservation race is unchanged from the base implementation.

Reviewed 1 of 1 changed files. Static review covered the changed scheduling loop, all findWorkerToRunTask call sites, built-in and JavaScript worker-selection strategies, WorkerHolder snapshot publication, reservation/assignment, and the existing review discussion.

Validation: git diff --check 0ad366d5d4a2bf3b1d35dd7f515c2302c5731e7e..30ceba0cdd930895b420f63e21564df9b1777f77 passed; the local git merge-tree --write-tree conflict check against origin/master passed. No tests, builds, dependency installs, formatters, or long validations were run.

This is an automated review by Codex GPT-5.6 Luna(Max)

FrankChen021 · 2026-10-10T09:18:35Z

I think aboves findings from the AI should be addressed. Under the case that when a worker disappears after the snapshot is built, current code fallback to previous behaviour, that's true, but here we are going to solve the performance problem here right?
and because the snapshot is built before each iteration, I think compared to the original code, it increases the probability that we will encounter above problem.

2nd, after this change, getWorkersEligibleToRunTasks can be called twice, it increases the overhead for a pending task scan that can find a worker.

Instead of patching the current pending executor path, another way is to track the tasks of a worker when its announcement changes. we can do it in the WorkerHolder#fullSync and WorkerHolder#deltaSync, and update the WorkerHolder#toImmutable to return the computed result directly instead of computing in the fly as current implementation.

Anubhav-Roy · 2026-10-14T13:23:30Z

@FrankChen021 Thanks for taking the time to review again!

As for the worker removal after snapshot is built, we still end up checking fresh at L:1158 before assigning it. The main purpose of this change is when we have a huge backlog of pending tasks, a sudden jump of tasks, into the queue. In the old implementation, it would end up locking for everyone and then keep computing which worker is free which ended up the cluster to stall.

For the second comment, it's still less than the current implementation ( same comment as above). For a backlog of tasks, this is where the current implementation shows it perf improvement when we don't compute for each task.

I'm sorry I didnt get the WorkerHolder approach. The problem we faced was this statusLock blocking for everyone. Could you please help clarify?

Thanks!

FrankChen021 · 2026-10-14T13:57:26Z

The stall comes from the work done under statusLock, not the lock itself.

Most of that work is WorkerHolder.toImmutable() → ImmutableWorkerInfo.fromWorkerAnnouncements(), which walks every task announcement on every worker — repeated for every pending task.

Instead of caching a snapshot in the pending loop, WorkerHolder could compute its ImmutableWorkerInfo when announcements change (at the end of fullSync / deltaSync, which run outside statusLock), store it in an AtomicReference, and have toImmutable() just return it.

The pending loop then stays unchanged and always sees current capacity, but snapshot building becomes O(workers) instead of O(workers × tasksPerWorker).

This removes both the stale-snapshot concern and the extra refresh cost, and also speeds up other toImmutable() callers.

lastCompletedTaskTime / blacklistedUntil changes would need to refresh the cached value too (or be attached at read time).

Anubhav-Roy · 2026-10-14T14:49:44Z

Aah okay. Now got it.

Yeah, that makes a lot more sense to compute it in the worker itself instead of proxying it in the pending loop.

FrankChen021

🟡 Changes recommended

The eager cached-view initialization can throw when a worker rejoins with placeholder announcements that intentionally have null task resources. The cached per-task-type map is also shared with returned ImmutableWorkerInfo instances, allowing callers to corrupt future scheduling snapshots.

Reviewed 3 of 3 changed files; findings in 1 file. Coverage included the current three-file diff, worker rejoin construction, sync callback publication, all WorkerHolder snapshot call sites, ImmutableWorkerInfo, and related tests. The supplied incremental diff was inconsistent with the packet and prepared worktree; the verified current-head diff contains three changed files.

Validation: git diff --check 7c89096bd1faefb14a4efef1cbd684719bf2aa83..c398d09a9f9a079c21acfc87d860dccccd3bc9d7 passed. No tests or builds were run.

Severity	Findings
P0	0
P1	1
P2	1
P3	0
Total	2

This is an automated review by Codex GPT-5.6-Luna(max)

After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.

FrankChen021 · 2026-10-14T15:16:05Z

      knownAnnouncements.forEach(e -> announcements.put(e.getTaskId(), e));
    }
    tasksSnapshotRef = new AtomicReference<>(announcements);
+    refreshAnnouncementsView();


[P1] Avoid deriving capacity from null-resource placeholders

Finding: When a worker rejoins while the runner still tracks a running task, HttpRemoteTaskRunner seeds this holder with a placeholder TaskAnnouncement whose TaskResource is null. Because the task is RUNNING, the eager refresh here passes it to ImmutableWorkerInfo.fromWorkerAnnouncements, which dereferences the resource to compute capacity and throws a NullPointerException during worker creation. The worker cannot rejoin and its task state cannot be reconciled.

Suggestion: Skip null-resource placeholder announcements when building the cached capacity view, or defer/otherwise guard this derivation until a real worker sync supplies complete task resources.

This isn't possible. The TaskAnnouncement constructor has -> new TaskResource(taskId, 1) when the resource is null, so getTaskResource() is never null.

Even then, I added testWorkerRejoiningWithPlaceholderAnnouncements

FrankChen021 · 2026-10-14T15:16:05Z

-        tasksSnapshotRef.get(),
+        announcementsView.getCurrCapacityUsed(),
+        announcementsView.getCurrParallelIndexCapacityUsed(),
+        announcementsView.getCurrCapacityUsedByTaskType(),


[P2] Do not share the cached mutable task-type map

Finding: This passes the cached view's task-type map directly into each returned ImmutableWorkerInfo. ImmutableWorkerInfo retains non-null maps as-is and exposes the map through getCurrCapacityUsedByTaskType(), so a caller that mutates a returned worker snapshot also mutates the cached view; subsequent snapshots and worker-selection decisions then see corrupted per-type counts. Before this cache, each toImmutable() call rebuilt an independent map.

Suggestion: Publish an immutable copy of the task-type map in the cached view or copy it when constructing each returned worker snapshot.

Fair callout. fixed.

toImmutable() now returns the task-type map wrapped in Collections.unmodifiableMap. Also covered by testToImmutableDoesNotExposeCachedCapacityByTaskType.

…pRemoteTaskRunner

…ot before assigning

…ing a snapshot in the pending loop

…ng workers in WorkerHolderTest

FrankChen021

Follow-up assessment

Both supplied review threads are resolved at the current head. TaskAnnouncement guarantees a non-null task resource even when constructed with null, and the new rejoin test covers that behavior. The cached task-type capacity map is now unmodifiable, with a regression test confirming callers cannot mutate cached counts.

No additional PR-caused findings remain, and no inline reply is warranted.

Reviewed 3 of 3 changed files.

This is an automated review by Codex GPT-5.6-Luna(max)

github-actions Bot added the Area - Ingestion label Sep 8, 2026

FrankChen021 reviewed Sep 9, 2026

View reviewed changes

Comment thread ...xing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java Outdated

Comment thread ...xing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java Outdated

FrankChen021 reviewed Sep 14, 2026

View reviewed changes

Anubhav-Roy requested a review from FrankChen021 September 15, 2026 14:01

FrankChen021 reviewed Sep 15, 2026

View reviewed changes

Comment thread ...xing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java

Comment thread ...xing-service/src/main/java/org/apache/druid/indexing/overlord/hrtr/HttpRemoteTaskRunner.java Outdated

Anubhav-Roy changed the title ~~Avoid rebuilding the eligible-worker snapshot per pending task in Htt…~~ avoid rebuilding the eligible-worker snapshot per pending task in HttpRemoteTaskRunner Sep 16, 2026

FrankChen021 reviewed Sep 17, 2026

View reviewed changes

Anubhav-Roy requested a review from FrankChen021 September 17, 2026 16:21

Anubhav-Roy changed the title ~~avoid rebuilding the eligible-worker snapshot per pending task in HttpRemoteTaskRunner~~ perf: avoid rebuilding the eligible-worker snapshot per pending task in HttpRemoteTaskRunner Sep 22, 2026

Anubhav-Roy closed this Sep 23, 2026

Anubhav-Roy reopened this Sep 23, 2026

Anubhav-Roy marked this pull request as draft September 23, 2026 13:08

Anubhav-Roy marked this pull request as ready for review September 23, 2026 13:09

Anubhav-Roy force-pushed the hrtr-avoid-rebuilding-worker-snapshot branch from a326c03 to 30ceba0 Compare September 30, 2026 09:20

FrankChen021 reviewed Sep 30, 2026

View reviewed changes

Anubhav-Roy requested a review from FrankChen021 October 5, 2026 12:41

Anubhav-Roy force-pushed the hrtr-avoid-rebuilding-worker-snapshot branch from 30ceba0 to c398d09 Compare October 14, 2026 14:57

FrankChen021 reviewed Oct 14, 2026

View reviewed changes

Anubhav Roy added 2 commits October 15, 2026 11:30

Avoid rebuilding the eligible-worker snapshot per pending task in Htt…

af69486

…pRemoteTaskRunner

Address review: build snapshot lazily and re-select on a fresh snapsh…

555a5d5

…ot before assigning

Anubhav Roy added 2 commits October 15, 2026 11:30

Cache WorkerHolder capacity view on announcement sync instead of reus…

9b9c75e

…ing a snapshot in the pending loop

Make the cached task-type capacity map unmodifiable and cover rejoini…

c2daec7

…ng workers in WorkerHolderTest

Anubhav-Roy force-pushed the hrtr-avoid-rebuilding-worker-snapshot branch from c398d09 to c2daec7 Compare October 15, 2026 11:33

FrankChen021 reviewed Oct 15, 2026

View reviewed changes

Anubhav-Roy requested a review from FrankChen021 October 19, 2026 09:21

Conversation

Anubhav-Roy commented Sep 8, 2026

Description

Fixed the Overlord stall on a large pending-task backlog

Release note

Key changed/added classes in this PR

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Follow-up assessment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Anubhav-Roy commented Sep 16, 2026

Uh oh!

FrankChen021 commented Sep 16, 2026

Uh oh!

Anubhav-Roy commented Sep 16, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Follow-up assessment

Uh oh!

Anubhav-Roy commented Sep 22, 2026

Uh oh!

FrankChen021 commented Sep 30, 2026

Uh oh!

Anubhav-Roy commented Sep 30, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟢 Approval recommended

Uh oh!

FrankChen021 commented Oct 10, 2026

Uh oh!

Anubhav-Roy commented Oct 14, 2026

Uh oh!

FrankChen021 commented Oct 14, 2026

Uh oh!

Anubhav-Roy commented Oct 14, 2026

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

FrankChen021 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Anubhav-Roy Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

FrankChen021 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Anubhav-Roy Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

FrankChen021 left a comment

Choose a reason for hiding this comment

Follow-up assessment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants